@gnprice Thoughts on testing setup for this?

Because ApiConnection depends on userAgent function, and it depends on ZulipBinding to get package & device info — now every test that uses ApiConnection will need to also first call TestZulipBinding.ensureInitialized().

Alternative implementation is to make ApiConnection take userAgent in it's constructor, which would just move this reposnibility to the upper layer (LiveGlobalStore for the real impl), but that would remove the need for TestZulipBinding.ensureInitialized() in each test.

Hmm yeah, having to do that in all the tests would be annoying.

It looks like there's only a small number of call sites of the ApiConnection constructors. So that's probably a good solution.

Hmm after trying the alternative I suggested, ended up reverting it, because it led to a bit of constructor argument poisoning up the chain of ApiConnection — making it harder to read the code where some constructors calls take the userAgent value, and some leave it out (null).

Instead ended up refactoring first implementation by moving the userAgent function to class ZulipBinding to make it explicit that it now depends on members in ZulipBinding and thus the binding needs to be initialized to get user-agent.

What were the callers that that propagated up the call chain to? I was imagining it'd be just that ApiConnection.live requires a real user-agent, and FakeApiConnection defaults to a boring fake one, and then very little other code has to get involved.

The implementation I tried was to keep deviceInfo and packageInfo as async function instead of the current sync getters on ZulipBinding and then trying to populate the ApiConnection args (deviceInfo & packageInfo) from the async functions it's being called. But there was one call which was not async, so had to add the args for it too. And doing so led to many other changes in the codebase which were moot, thus reverted it.

But if we stick to sync function userAgentHeader and let ApiConnection.live inline it in it's constructor like:

ApiConnection.live({
  ...
}) : this(client: http.Client(),
          ...
          userAgentHeader: ZulipBinding.instance.userAgentHeader());

then it does work without the need to poison the callers, I'll send a revision with this now.

Okay just tried it, and seems like there isn't much difference in line count, because now some tests that needs to check userAgent needs to pass the testBinding.userAgent to FakeApiConnection.with_ which are quite a few.

But on the plus side, the tests that don't check userAgent don't need to add ZulipBinding initialization lines:

-  TestZulipBinding.ensureInitialized();
-  tearDown(testBinding.reset);

Okay just tried it, and seems like there isn't much difference in line count, because now some tests that needs to check userAgent needs to pass the testBinding.userAgent to FakeApiConnection.with_ which are quite a few.

Cool. Yeah, even if the total line count is the same but the impact is all concentrated in the one group of tests that's about the user-agent value itself, that sounds a lot nicer.

We can probably also improve that further with a local helper or two in those tests.

The goal for this change is to pre-fetch the deviceInfo and packageInfo while ZulipBinding is being initialized and then later they can be used from non-async functions via the getters.

That make sense from an ergonomics perspective; but it makes me nervous to be introducing this wait into the startup path. Probably on any reasonable system the queries will return quickly and it's fine, but

• there may be bugs in these third-party plugins that cause them to sometimes hang before supplying an answer;
• there may be devices out there where some bug in a vendor's low-quality modifications to the Android platform software cause the queries to hang, or just to be slow, even though the plugins are doing only perfectly reasonable things that totally ought to be reliably fast.

Then the thing about putting awaits like these into the startup path is that it'd escalate any bugs of those types into a serious problem in the Zulip app for affected users.

So let's arrange things instead so that if for some reason those queries are slow, or even hang forever and never return, everything fails gracefully — just the places that actually wanted to use the answers have to make do with not knowing the answers.

One good way to do that, API-wise, would be to indeed make those into nice non-async getters, but returning nullable. Then when the binding is initialized, it kicks off the queries, and if the queries have finished by the time we try to make a request then we use the results; if not, we fall back to not knowing this information.

I guess most of the time we'll be firing off one request promptly after startup, the register-events request. So if we find that that often happens before the platform queries finish, then we can add an extra wrinkle where we wait for the queries to finish… but with a timeout, like 10ms or something, so we prioritize the user's experience over being sure we include this information.

Let's keep that as a separate followup commit, though. Or it'd be great if you try a little manual experimenting to see if that's a problem in practice; if it seems like it isn't, we can skip that wrinkle entirely, and keep it in our back pocket as a possible followup for later if it turns out to be a problem sometimes in wider deployment.

Thanks, yeah we definitely should have a fail safe if plugins fails for some reason or are slow to retrieve the data.

The latest revision now doesn't await on pre-fetchs for both (deviceInfo and packageInfo), and where the getters are called there's now some kind of relevant fallback just-in-case.

if you try a little manual experimenting to see if that's a problem in practice

I tried that, by throw-ing in case either of infos are null but didn't see any exceptions in both release and debug builds for Android & macOS (can't try release on iOS Simulator) during startup.

I think we can remove null => true.

How about adding this change in its separate commit?!

I think it will also be nice to add the classes for these new platforms and the related changes to LiveZulipBinding._prefetchDeviceInfo in a separate commit too!

api: Embed platform and app info in user-agent

After adding the previous changes to their separate commits, now this commit can focus on embedding platform and app info in the user-agent as the commit message suggests.

nit: Can be inlined.

Do these lines need to be reordered?

Yes they need to be reordered, since deviceInfo is being prefetched in LiveZulipBinding.ensureInitialized() and PlatformChannels require WidgetsFlutterBinding to be initialized first.

What does the error look like if the order of these gets swapped back?

If it doesn't make clear that the problem is that WidgetsFlutterBinding is needed first, then ideally we can do something to make the error clear.

If that's hard or annoying, then just a comment here explaining this ordering constraint would also be an acceptable solution.

Here's how the errors look like:

flutter: Failed to prefetch device info: Binding has not yet been initialized.
The "instance" getter on the ServicesBinding binding mixin is only available once that binding has been initialized.
Typically, this is done by calling "WidgetsFlutterBinding.ensureInitialized()" or "runApp()" (the latter calls the former). Typically this call is done in the "void main()" method. The "ensureInitialized" method is idempotent; calling it multiple times is not harmful. After calling that method, the "instance" getter will return the binding.
In a test, one can call "TestWidgetsFlutterBinding.ensureInitialized()" as the first line in the test's "main()" method to initialize the binding.
If ServicesBinding is a custom binding mixin, there must also be a custom binding class, like WidgetsFlutterBinding, but that mixes in the selected binding, and that is the class that must be constructed before using the "instance" getter.
#0      BindingBase.checkInstance.<anonymous closure> (package:flutter/src/foundation/binding.dart:309:9)
#1      BindingBase.checkInstance (package:flutter/src/foundation/binding.dart:390:6)
#2      ServicesBinding.instance (package:flutter/src/services/binding.dart:68:54)
#3      _findBinaryMessenger (package:flutter/src/services/platform_channel.dart:158:25)
#4      MethodChannel.binaryMessenger (package:flutter/src/services/platform_channel.dart:293:56)
#5      MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:327:15)
#6      MethodChannel.invokeMethod (package:flutter/src/services/platform_channel.dart:507:12)
#7      MethodChannelDeviceInfo.deviceInfo (package:device_info_plus_platform_interface/method_channel/method_channel_device_info.dart:19:24)
#8      DeviceInfoPlugin.macOsInfo (package:device_info_plus/device_info_plus.dart:86:48)
#9      DeviceInfoPlugin.deviceInfo (package:device_info_plus/device_info_plus.dart:107:16)
#10     LiveZulipBinding._prefetchDeviceInfo (package:zulip/model/binding.dart:338:62)
#11     new LiveZulipBinding (package:zulip/model/binding.dart:290:19)
#12     LiveZulipBinding.ensureInitialized (package:zulip/model/binding.dart:297:7)
#13     main (package:zulip/main.dart:16:20)
#14     _runMain.<anonymous closure> (dart:ui/hooks.dart:301:23)
#15     _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#16     _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

flutter: Failed to prefetch package info: Binding has not yet been initialized.
The "instance" getter on the ServicesBinding binding mixin is only available once that binding has been initialized.
Typically, this is done by calling "WidgetsFlutterBinding.ensureInitialized()" or "runApp()" (the latter calls the former). Typically this call is done in the "void main()" method. The "ensureInitialized" method is idempotent; calling it multiple times is not harmful. After calling that method, the "instance" getter will return the binding.
In a test, one can call "TestWidgetsFlutterBinding.ensureInitialized()" as the first line in the test's "main()" method to initialize the binding.
If ServicesBinding is a custom binding mixin, there must also be a custom binding class, like WidgetsFlutterBinding, but that mixes in the selected binding, and that is the class that must be constructed before using the "instance" getter.
#0      BindingBase.checkInstance.<anonymous closure> (package:flutter/src/foundation/binding.dart:309:9)
#1      BindingBase.checkInstance (package:flutter/src/foundation/binding.dart:390:6)
#2      ServicesBinding.instance (package:flutter/src/services/binding.dart:68:54)
#3      _findBinaryMessenger (package:flutter/src/services/platform_channel.dart:158:25)
#4      MethodChannel.binaryMessenger (package:flutter/src/services/platform_channel.dart:293:56)
#5      MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:327:15)
#6      MethodChannel.invokeMethod (package:flutter/src/services/platform_channel.dart:507:12)
#7      MethodChannel.invokeMapMethod (package:flutter/src/services/platform_channel.dart:534:49)
#8      MethodChannelPackageInfo.getAll (package:package_info_plus_platform_interface/method_channel_package_info.dart:13:32)
#9      PackageInfo.fromPlatform (package:package_info_plus/package_info_plus.dart:80:61)
#10     LiveZulipBinding._prefetchPackageInfo (package:zulip/model/binding.dart:367:56)
#11     new LiveZulipBinding (package:zulip/model/binding.dart:291:20)
#12     LiveZulipBinding.ensureInitialized (package:zulip/model/binding.dart:297:7)
#13     main (package:zulip/main.dart:16:20)
#14     _runMain.<anonymous closure> (dart:ui/hooks.dart:301:23)
#15     _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#16     _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

Basically both of the prefetch fail because of same reason — MethodChannel requiring WidgetsFlutterBinding initialized first.

Cool, that seems clear enough — thanks.

Can we explain the meaning of deviceInfo == null in a dartdoc on the deviceInfo getter? That seems more efficient than explaining it separately at each call site that handles the null case. Curious readers can look up the dartdoc to learn what it means.

Then I think 1., and the rest of this comment, can be removed too. I think the comment "Android 13+ shows its own popup…", above this, implies that other platforms don't show a popup.

binding [nfc]: Refactor deviceInfo to a getter

Let's highlight that we're changing the logic so that the device info is prefetched. And since this seems like a behavior change (though a small one), let's not mark it NFC.

The _prefetchPackageInfo call isn't awaited in LiveZulipBinding.ensureInitialized, so that method completes before the info is fetched and stored. It may be true that "ZulipBinding should already be initialized", but that doesn't seem sufficient to ensure that we've waited long enough to get success or failure from the fetch.

For this screen, I think we're content with the existing behavior: show the info if we have it, and just show a placeholder if we don't.

This is a behavior change, so let's leave it out of a pure-refactor [nfc] commit. Before, when the app version is undetermined, we'd show a ListTile with subtitle "(…)". Now, in that case, the whole ListTile is omitted.

removed this behavior change

I think this is a one-line change plus a reformatting with different whitespace; is that right? The whitespace change is kind of distracting to me as I review looking for meaningful changes. (Here and in several places below.)

It looks like the whitespace changes in question are all an indentation change. It's natural for those to happen together with substantive changes sometimes — for example they'll happen whenever adding or removing a wrapper widget around a child subtree.

A handy tool for reading such diffs is git log -b, which ignores indentation changes. With that, this diff hunk looks like:

-      return FakeApiConnection.with_(account: eg.selfAccount, (connection) async {
+      return FakeApiConnection.with_(account: eg.selfAccount,
+        userAgentHeader: testBinding.userAgentHeader(),
+        (connection) async {
           connection.prepare(json: {});
           await connection.get(kExampleRouteName, (json) => json, 'example/route', params);
           check(connection.lastRequest!).isA<http.Request>()
             ..method.equals('GET')
             ..url.asString.equals('${eg.realmUrl.origin}$expectedRelativeUrl')
             ..headers.deepEquals({
               ...authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey),
-            ...userAgentHeader(),
+              ...testBinding.userAgentHeader(),
             })
             ..body.equals('');

(With git config diff.colorMovedWS, you can also get your default git log -p output to treat indentation changes the same way as moved code and give them a different background color. That's what I have in my gitconfig. I still use -b to get a clearer view in extreme cases.)

Cool. Yeah; I used git log -b to help verify the change, but I think I could have looked closer at the regular git log -p output—it turns out I already have that git config option set the way you do, and here's what it looks like:

So, noticing that a lot of the text has the "moved" colors (magenta and a more bluish-green), and also the faint background color, would have been helpful. That background color does end up being pretty faint on my screen; I bet there's a way to adjust it.

Yeah, exactly. And yeah, those colors are adjustable - they're the diff.oldMoved and friends found in my gitconfig. Definitely worth trying some quick changes to them to see what works best for you.

From the screenshot, it looks like your terminal's default background color is already a faint gray rather than black, which ends up being pretty close to the background colors set there. I have my terminal background set to pure black, for maximum contrast, and then the colors in my config are chosen to be distinct from that.

This is better—I've changed the Android Studio color scheme to "high contrast":

Yeah, here's what that looks like for me:

The striping in your screenshot looks like it could get a bit annoying. You might be able to adjust the line spacing to fix that — generally I'd expect a terminal to use 1.0x line spacing anyway.

The striping in your screenshot looks like it could get a bit annoying. You might be able to adjust the line spacing to fix that — generally I'd expect a terminal to use 1.0x line spacing anyway.

Yep, got it! I adjusted a font setting in the Android Studio settings. It was a little confusing because things outside the terminal were responding to the setting, but the terminal wasn't—but then it did after I restarted Android Studio.

It would be cleanest/most principled to add a line

addTearDown(testBinding.reset);

at the top of the test, like we do in other tests. (But I don't think the lack of the line causes a problem with this test as written.)

There is a addTearDown(testBinding.reset) at the start of the main, which should work for all tests in that file.

Oh! Indeed; thanks for pointing this out.

Cool! The way these cases is presented is nice and compact and easy to read, without lots of extra boilerplate for each case.

(Ah I did just notice there's a typo "pacakgeInfo" a few lines below this.)

This is a bit more concise, and I think the success case is clear from the return type (Future<BaseDeviceInfo?>). Callers might wonder what happens in the error case, though (does it reject, or resolve to null), so it's good to keep the mention of that.

Also, maybe good to put this after this line, instead of before it:

/// This wraps [device_info_plus.DeviceInfoPlugin.deviceInfo].

since it's now just about what happens in the error case, which should be unusual.

(similarly with packageInfo below)

Addressed, though removed This wraps ... because the getter don't really wrap those functions anymore.

Ah, and I didn't notice last time: is there a reason to move this device_info_plus-related code before other things in the class? I think ZulipBinding, LiveZulipBinding, and TestZulipBinding should declare their members in the same order; the consistency should help reading and editing them together.

Same question 🙂

What does the error look like if the order of these gets swapped back?

If it doesn't make clear that the problem is that WidgetsFlutterBinding is needed first, then ideally we can do something to make the error clear.

If that's hard or annoying, then just a comment here explaining this ordering constraint would also be an acceptable solution.

nit: this logic is probably cleaner to read as a constructor body

Same question 🙂

nit: keep "device info" members next to "device info" members, and "package info" members next to "package info" members

More generally, as the Flutter style guide puts it:

Fields should come before the methods that manipulate them, if they are specific to a particular group of methods.

where the key part of "before" is that they're next to those methods, though in particular they should come just before them.